Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14194 +/- ##
============================================
+ Coverage 18.01% 18.13% +0.11%
- Complexity 16265 16412 +147
============================================
Files 5936 5937 +1
Lines 536395 537271 +876
Branches 65701 65857 +156
============================================
+ Hits 96658 97419 +761
- Misses 428710 428742 +32
- Partials 11027 11110 +83
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@rp- are the changes here relavant for 4.22 branch? if so, please re-target to 4.22. |
thanks, missed the wrong base branch |
|
@blueorangutan package |
|
@rp- a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19402 |
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19405 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
Also pinging for this here @sureshanaparti to be included in 4.22.2 |
|
[SF] Trillian test result (tid-17075)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
1 similar comment
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
The incremental NAS backup feature gates on QEMU dirty bitmaps, which can only be persisted inside a qcow2. LINSTOR volumes are raw DRBD block devices, so allVolumesOnCheckpointCapableStorage() rejected them and every LINSTOR VM was pinned to full-only backups regardless of the configured cadence. Route those VMs to the content-based path instead, which derives the delta by comparing the disk's point-in-time NBD export against the parent backup rather than by tracking dirty blocks: - decideContentChain() mirrors the bitmap cadence logic (master switch, stopped VMs, nas.backup.full.every, chain length) but anchors on backup history alone. There is no host-side bitmap to invalidate, so unlike the bitmap chain a VM restart, live migration or restore cannot break it -- content comparison is stateless and self-correcting. - The path is taken even when the incremental master switch is off, because a content-full is still a single-pass sparse write, strictly better than the fully allocated push-mode output it replaces. - ChainDecision carries the parent backup uuid directly, since a content chain has no bitmap name to resolve the parent by. - Mixed-storage VMs stay off the content path: one backup run uses one mode. The agent wrapper accepts the two new modes -- content-incremental requires only its per-disk parent paths, no bitmap -- and passes the host-local pull-mode scratch directory from the new nas.backup.pull.scratch.dir agent property. Adds 6 unit tests covering the routing and cadence decisions. All 21 pre-existing NASBackupProviderTest cases and the KVM wrapper tests still pass.
A pull-mode backup job never finishes on its own; nasbackup.sh ends it with domjobabort once the data is read. Any exit the script did not plan for -- an unexpected error under "set -e", SIGINT/SIGTERM/SIGHUP -- left the job running, keeping the NBD server and the growing fleecing images alive and blocking every later backup or migration of the VM. Arm an EXIT trap once the scratch dir exists: it aborts the job (through apache#14305's BACKUP_JOB_ACTIVE/abort_backup_job), removes the scratch dir and runs cleanup(). This replaces the per-error-path abort/cleanup copies. The agent kills a timed-out script with SIGKILL, which no trap can catch, so LibvirtTakeBackupCommandWrapper aborts the job itself after a pull-mode timeout.
The content-based path overloaded ChainDecision.mode with content-full and content-incremental, and apache#14386 is adding rbd-full and rbd-incremental the same way. Split the two concerns as suggested in review: mode stays full or incremental, and a new method field says how the backup is taken -- qcow2 (push mode with dirty bitmaps) or content (pull mode). Further methods such as an RBD diff can be added without new mode tokens. TakeBackupCommand gets a method field and nasbackup.sh a --method option. Both default to qcow2 when absent, so a management server that does not send a method keeps working against an upgraded agent. legacy-full is unchanged: rewriting it as full plus a "none" method would break incremental-disabled backups on agents that have not been upgraded yet.
Copy-before-write saves every guest block that is overwritten during a pull-mode backup into the host-local scratch area. If that fills up, the guest's writes fail, QEMU pauses the VM (the default werror=enospc policy) and nothing resumes it. Each block is saved at most once, so the VM's total disk size is the worst case. Refuse to start the backup unless the scratch area has the share of that set by the new agent.properties option nas.backup.pull.scratch.min.free.percent. The default of 25 assumes the guest overwrites at most a quarter of its disks while a backup runs; 100 rules out a full scratch area, 0 disables the check. The check does not account for concurrent pull-mode backups sharing the same scratch filesystem.
… off LINSTOR VMs took the content-based pull-mode path even with nas.backup.incremental.enabled off, so an upgrade switched existing users to the new mechanism, with its host-local scratch space requirement, without them opting in. Apply the master switch before the storage routing: with incrementals disabled every VM, LINSTOR included, gets the legacy push-mode full as before. Enabling incrementals is the opt-in to the content-based path.
replaceBlockDeviceWithBackup created its QemuImg with skipZeroIfSupported, so for an existing target device qemu-img convert ran with --target-is-zero and skipped every zero region of the backup -- at qcow2 cluster granularity, not the -S 1M chunk size. On an in-place restore the volume keeps its old data there, silently corrupting the restored disk. A newly created LINSTOR volume is only safe on thin or ZFS backed pools; thick LVM hands out stale data too. Add KVMStoragePool.newVolumeReadsAsZero(), false by default; LinstorStoragePool answers it with LinstorUtil.resourceSupportZeroBlocks(), the check LinstorStorageAdaptor.copyPhysicalDisk already uses. The restore now skips zero regions only on a volume it just created and the pool reports as zeroed, and requests -n explicitly for an existing target, which it previously got only through the zero-skipping branch. RBD targets are unaffected: --target-is-zero never applied to them.
d509516 to
1d9b64c
Compare
The RBD path overloaded ChainDecision.mode with rbd-full and rbd-incremental, the same way the content-based path upstream overloaded it with content-full and content-incremental. Split the two concerns as that path already did: mode stays full or incremental, and a new method field says how the backup is taken -- qcow2 (push mode with dirty bitmaps) or rbd (Ceph-native, anchored on an rbd snapshot and exported with rbd export-diff). TakeBackupCommand gets a method field and nasbackup.sh a --method option. Both default to qcow2 when absent, so a management server that does not send a method keeps working against an upgraded agent. legacy-full is unchanged: rewriting it as full plus a "none" method would break incremental-disabled backups on agents that have not been upgraded yet. CHAIN_FORMAT stays as it is. The method says how a backup is taken and lives only for the duration of the request; the chain format is persisted and says what is on the repository, which is what restore and delete read later. The two coincide for the rbd method today but answer different questions. This mirrors c144d09 from apache#14194, which introduced the method for the content path and named an RBD diff as the next one to adopt it. Kept as close to that commit as the branch allows so the two merge cleanly: this branch carries the RBD path but not the content one, so the METHOD_CONTENT token is defined without a user here.
The RBD path overloaded ChainDecision.mode with rbd-full and rbd-incremental, the same way the content-based path upstream overloaded it with content-full and content-incremental. Split the two concerns as that path already did: mode stays full or incremental, and a new method field says how the backup is taken -- qcow2 (push mode with dirty bitmaps) or rbd (Ceph-native, anchored on an rbd snapshot and exported with rbd export-diff). TakeBackupCommand gets a method field and nasbackup.sh a --method option. Both default to qcow2 when absent, so a management server that does not send a method keeps working against an upgraded agent. legacy-full is unchanged: rewriting it as full plus a "none" method would break incremental-disabled backups on agents that have not been upgraded yet. CHAIN_FORMAT stays as it is. The method says how a backup is taken and lives only for the duration of the request; the chain format is persisted and says what is on the repository, which is what restore and delete read later. The two coincide for the rbd method today but answer different questions. This mirrors c144d09 from apache#14194, which introduced the method for the content path and named an RBD diff as the next one to adopt it. Kept as close to that commit as the branch allows so the two merge cleanly: this branch carries the RBD path but not the content one, so the METHOD_CONTENT token is defined without a user here.
The RBD path overloaded ChainDecision.mode with rbd-full and rbd-incremental, the same way the content-based path upstream overloaded it with content-full and content-incremental. Split the two concerns as that path already did: mode stays full or incremental, and a new method field says how the backup is taken -- qcow2 (push mode with dirty bitmaps) or rbd (Ceph-native, anchored on an rbd snapshot and exported with rbd export-diff). TakeBackupCommand gets a method field and nasbackup.sh a --method option. Both default to qcow2 when absent, so a management server that does not send a method keeps working against an upgraded agent. legacy-full is unchanged: rewriting it as full plus a "none" method would break incremental-disabled backups on agents that have not been upgraded yet. CHAIN_FORMAT stays as it is. The method says how a backup is taken and lives only for the duration of the request; the chain format is persisted and says what is on the repository, which is what restore and delete read later. The two coincide for the rbd method today but answer different questions. This mirrors c144d09 from apache#14194, which introduced the method for the content path and named an RBD diff as the next one to adopt it. Kept as close to that commit as the branch allows so the two merge cleanly: this branch carries the RBD path but not the content one, so the METHOD_CONTENT token is defined without a user here.
|
@abh1sar |
abh1sar
left a comment
There was a problem hiding this comment.
I have pointed some performance concerns
I am ok with the PR being merged as it is now since the feature is opt-in, but the issues should be contemplated and addressed in a follow up PR or mentioned clearly in documentation.
Test results
|
…e agent log backup_stopped_vm redirected qemu-img convert's stdout with ">" into $logFile, which is the agent's own agent.log, so every backup of a stopped VM truncated the agent log, taking along whatever the agent had logged about the backup until then. Append instead, as every other qemu-img call in the script already does. When the convert failed, the script ran cleanup -- removing the backup directory and unmounting the share -- but then carried on with the next disk. Its convert and the second cleanup failed in turn, so the backup ended with "Backup cleanup failed" (exit 20) and was left in Error state although nothing needed cleaning up. Exit after the cleanup, as the other failure paths do.
createPhysicalDisk and connectPhysicalDisk return as soon as LINSTOR has made the resource available on this node, but udev creates its /dev/drbd/by-res link a little later. A caller that opens the device right away fails with "Could not open '/dev/drbd/by-res/cs-<uuid>/0': No such file or directory". Restoring a backup onto LINSTOR runs qemu-img convert onto the volume immediately: creating an instance from a backup hit this on every new volume, an in-place restore whenever the host had no replica of the volume. Look up the resource's device on this node after making it available and wait up to 10 seconds for it to appear.
A stopped VM is backed up by qemu-img convert straight from its volume devices, on the VM's last host. A LINSTOR volume only has a device on hosts that hold a resource of it, and when the VM stops CloudStack removes the diskless resource it ran on. Unless that host also holds a replica, the convert fails with "Could not open '/dev/drbd/by-res/cs-<uuid>/0': No such file or directory" -- on a hyperconverged setup sometimes, on one with separate compute and storage nodes always. Before running the script, make every LINSTOR volume whose device does not exist on this host available -- connecting returns once its device exists -- and disconnect it again once the backup is done, failed or not. Volumes that already have a device -- those of a running VM, or ones this host holds a replica of -- are left alone, so a running VM's disks are never touched.
Copy-before-write kept the guest data a pull-mode backup had not read yet in fleecing files under a host-local directory (/var/tmp by default). Their growth is bounded only by the guest's writes during the backup, so the script refused to start unless that filesystem had a configurable share of the VM's disk size free; if it filled up anyway the guest's writes failed and QEMU paused the VM. Concurrent backups shared the same filesystem unchecked. Instead, the agent now creates one LINSTOR resource per backup, with one volume per disk, sized for the whole disk plus qcow2 metadata, and passes the devices to nasbackup.sh with --scratch-devs. libvirt formats them as the block scratch of each disk (<scratch dev=.../>), so a scratch device can no longer fill up. The agent deletes the resource once the backup has ended. - The resource definition is created in the pool's resource group, so the group's properties and storage pool selection apply. It is placed on a storage pool of the VM's host as a plain STORAGE resource if there is one; otherwise, e.g. on a compute node without storage, it is a single DRBD replica elsewhere that the host attaches diskless. - KVMStoragePool gets createBackupScratch()/deleteBackupScratch(); pools that cannot provide scratch space fail the pull-mode backup. There is no fallback to host-local files. - The management server now also sends the volume pools for content-method backups of running VMs, so the agent knows where to take the scratch from. - A resource left behind by a crashed agent has to be deleted manually; its Aux/cs-backup-vm property names the VM. The agent.properties options nas.backup.pull.scratch.dir and nas.backup.pull.scratch.min.free.percent, the free-space check and the -S and --scratch-min-free-percent script options are removed. The NBD socket and the backup XML stay in a small work directory under /var/tmp.
The agent deletes the LINSTOR scratch resource of a pull-mode backup once the backup has ended, also when the backup fails or the agent is stopped normally. If the agent dies or the host goes down during the backup, the resource stays behind and keeps its storage allocated. When the agent registers the first pool of a LINSTOR controller, delete every cs-bkscratch-* resource definition that has a resource on this node: a scratch always has one on the node whose agent created it, diskless if its data lives elsewhere. Pools are registered again on every reconnect to the management server, while backups may still be running, so this happens only once per controller and agent process, before any backup can use the pool. Failures are only logged and never keep the pool from being registered. After a hard crash the VM's pull-mode backup job can outlive the agent and still hold the scratch devices, so LINSTOR refuses the delete. The warning then says to end the job with virsh domjobabort and delete the resource definition manually.
abh1sar
left a comment
There was a problem hiding this comment.
LGTM. reviewed and tested
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✖️ suse15. SL-JID 19641 |
Description
This PR adds incremental NAS backup support for KVM VMs whose disks live on raw block-device storage such as LINSTOR/DRBD.
#13877 (backport of #13074) implements incremental NAS backups with QEMU dirty bitmaps and libvirt checkpoints. Bitmap persistence is a qcow2-only feature, so
allVolumesOnCheckpointCapableStorage()excludes raw block devices and every LINSTOR VM stays pinned to full-only backups regardless of the configured cadence. On top of that, the push-mode full backup of a raw device writes a fully allocated qcow2 which then needs a secondqemu-img convertpass to reclaim space.This PR adds a content-based backup path driven by libvirt pull mode and routes VMs on content-diff-capable storage (currently LINSTOR) to it:
nasbackup.sh– newcontentbackup methodThe script gets a
--methodoption next to-M|--mode:qcow2(default, the existing push-mode path with dirty bitmaps) orcontent. With--method content, libvirt is asked for a pull-mode backup: it starts an NBD server exposing a point-in-time view of each disk, held consistent by copy-before-write into a scratch block device per disk. The backup is then derived from that export:full:qemu-img convertfrom the NBD export straight into a sparse qcow2 in a single pass. This replaces the bloated push-mode output and its re-convert pass.incremental: a qcow2 overlay backed by the NBD export, then a safeqemu-img rebaseonto the parent backup. This merges in exactly the clusters that differ and yields a delta qcow2 backed by the parent, with a relative backing path. The chain has the same shape as a bitmap incremental, so the existing restore and cascade-delete logic applies unchanged.A missing or short parent list degrades to a full and emits
INCREMENTAL_FALLBACK=true, mirroring the push-mode fallback. Also handles the raw/dev/drbdNdevice paths of long-running VMs the same way the push/stopped paths do (viaget_linstor_uuid_from_device).The scratch devices are passed with
--scratch-devs, one per disk; libvirt formats them as qcow2 (<scratch dev=.../>). The NBD socket and the backup XML live in a small work directory under/var/tmp.A pull-mode job never ends on its own, so an EXIT/INT/TERM/HUP trap ends it (via
abort_backup_job()from #14305), removes the work directory and cleans up on any failed exit. The freeze/thaw handling uses the helpers from #14305 as well.Scratch space on the primary storage
Copy-before-write runs inline with the guest's writes and saves every block overwritten during the backup, at most once. The agent therefore creates one LINSTOR resource per backup (
cs-bkscratch-<uuid>), with one volume per disk sized for the whole disk plus qcow2 metadata, so the scratch can never fill up and pause the VM, also with several backups running at once:cs-bkscratch-*resource definition with a resource on its node. This happens once per controller and agent process: pools are registered again on every reconnect, while backups may still be running. After a hard crash the VM's pull-mode job can outlive the agent and still hold the scratch, so LINSTOR refuses the delete; the warning then names the VM (fromAux/cs-backup-vm) and says to end its job withvirsh domjobabortand delete the resource manually.KVMStoragePoolgetscreateBackupScratch()/deleteBackupScratch(), implemented byLinstorStoragePool. A pool that cannot provide scratch space fails the pull-mode backup; there is no fallback to host-local files.NASBackupProvider– routing and cadencedecideContentChain()mirrors the bitmap cadence logic (stopped VMs,nas.backup.full.every, chain length) but anchors on backup history alone. There is no host-side bitmap that can be invalidated, so a VM restart, live migration or restore cannot break the chain; content comparison is stateless and self-correcting.nas.backup.incremental.enabledapplies first: with incrementals disabled every VM, LINSTOR included, keeps the legacy push-mode full backup. Enabling incrementals is the opt-in to the content-based path.ChainDecisionseparates the two concerns:modeisfullorincremental, a newmethodfield isqcow2orcontent. Further methods (e.g. an RBD diff) can be added without new mode tokens.ChainDecisioncarries the parent backup uuid directly, since a content chain has no bitmap name to resolve the parent by.LibvirtTakeBackupCommandWrapper/ agentTakeBackupCommandcarries the newmethodfield, passed to the script as--method. When absent the agent assumesqcow2, so a management server that does not send it keeps working. Acontentincremental requires only its per-disk parent paths, no bitmap. No newagent.propertieskeys are needed.The agent kills a timed-out script with SIGKILL, which no trap can catch, so after a pull-mode timeout the wrapper aborts the backup job itself.
Restore onto LINSTOR: do not skip zero regions (bug fix, independent of the above)
replaceBlockDeviceWithBackupranqemu-img convertwith--target-is-zerowhenever the target device existed, skipping every zero/unallocated qcow2 cluster of the backup. On an in-place restore those clusters kept the volume's current data, and a new LINSTOR volume on thick LVM is not zeroed either. A newKVMStoragePool.newVolumeReadsAsZero()hook (false by default,LinstorStoragePoolanswers it withLinstorUtil.resourceSupportZeroBlocks()) now limits zero skipping to freshly created volumes on thin or ZFS backed pools. RBD targets were never affected.LINSTOR: wait for the device after making a resource available (bug fix)
createPhysicalDiskandconnectPhysicalDiskreturned as soon as LINSTOR had made the resource available on the node, but udev creates the/dev/drbd/by-reslink a little later. Restoring a backup runsqemu-img convertonto the volume right away and failed with "Could not open '/dev/drbd/by-res/cs-/0': No such file or directory": on every new volume when creating an instance from a backup, and on an in-place restore whenever the host held no replica. Both now wait up to 10 seconds for the local device.Stopped-VM backups (bug fixes)
backup_stopped_vmredirectedqemu-img convertwith>into$logFile, the agent's ownagent.log, truncating it on every stopped-VM backup. It now appends.cleanupbut carried on with the next disk, which then failed the second cleanup and left the backup in Error state ("Backup cleanup failed"). It now exits after the cleanup.Behaviour for VMs on qcow2/NFS/Ceph storage is unchanged; they keep using the bitmap path from #13877.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Unit tests
Seven new
NASBackupProviderTestcases cover the routing and cadence decisions and the volume pools sent for content-method backups:decideChainReturnsLegacyFullForLinstorWhenIncrementalDisableddecideChainReturnsContentFullForStoppedLinstorVmdecideChainReturnsContentIncrementalForLinstorWithValidParentdecideChainReturnsContentFullForLinstorAtChainEndallVolumesOnContentDiffCapableStorageFalseForMixedStorageallVolumesOnContentDiffCapableStorageFalseForNoVolumestakeBackupSendsVolumePoolsForContentMethodOnRunningVmLibvirtTakeBackupCommandWrapperTestcovers the--method/--scratch-devsarguments, scratch creation, sizing and cleanup (also when the backup fails or times out), the method validation, the job abort after a pull-mode timeout and connecting a stopped VM's LINSTOR volumes. The newLinstorStorageAdaptorTestcovers local STORAGE-only and remote diskless scratch placement, cleanup of a scratch that cannot be placed, the device wait increatePhysicalDisk/connectPhysicalDisk, and deleting leftover scratch on pool registration (only this node's, once per controller, never failing the registration).LibvirtRestoreBackupCommandWrapperTestcovers zero skipping for in-place, new thick and new thin LINSTOR restores.All
NASBackupProviderTest(30),LibvirtTakeBackupCommandWrapperTest(18),LibvirtRestoreBackupCommandWrapperTest(23) andLinstorStorageAdaptorTest(11) cases pass, as do the full agent, KVM plugin, LINSTOR plugin and NAS plugin suites.End-to-end on a 3-host KVM/LINSTOR cluster (management server + 3 agents, LINSTOR 1.35.2, libvirt 10.0, NFS backup repository,
nas.backup.incremental.enabled=true), on the current revision:agent.logintacttest/integration/smoke/test_backup_recovery_nas.pyagainst LINSTORTest 6 ran with the small
marvin-alpineguest template (see #13746) in a Basic zone. Creating an instance from a backup does not work in a Basic zone on current 4.22 for reasons outside this PR (it passes the backup's network ids, which a Basic zone rejects, and the new instance gets the account's default security group), so those were worked around locally for the run.How did you try to break this feature and the system with this change?
The first four were run on the current revision, the others on the initial one with host-local scratch files.
virsh domjobabortand deleting the resource the next backup succeeds.INCREMENTAL_FALLBACK=true, and the DB records a full.qemu-imgduring the pull): the backup job is aborted, scratch and partial output removed, mount released; the next backup succeeds. Deleting the LINSTOR scratch resource after a failed or timed-out backup is covered by the unit tests./dev/drbdNinstead of/dev/drbd/by-res/…in live XML): resolved via udev like the push path; if the UUID cannot be derived the backup fails hard rather than producing a file restore cannot find.